New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add contract to migrate a Safe from not L2 to L2 #685

Merged

Uxio0 merged 13 commits into main from migrate-l2

Oct 31, 2023

+526 −0

Member

Uxio0 commented Oct 19, 2023 •

edited

Loading

Add contract to migrate a Safe from not L2 to L2

Only allow Safes with nonce=0, so they are unitialized and the L2 indexer is not missing some configuration changes going on between the update to L2
Related to Support L2 Safes upgraded from non L2 Safes safe-transaction-service#1703

github-actions bot commented Oct 19, 2023

Pull Request Test Coverage Report for Build 6578252603

15 of 18 (83.33%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.7%) to 94.487%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/libraries/SafeToL2Migration.sol	15	18	83.33%

Totals
Change from base Build 6494000556:	-0.7%
Covered Lines:	383
Relevant Lines:	395

💛 - Coveralls

Uxio0 changed the title ~~migrate l2~~ Add contract to migrate a Safe from not L2 to L2

coveralls commented Oct 20, 2023 •

edited

Loading

Pull Request Test Coverage Report for Build 6626835905

Details

13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 94.907%

Totals
Change from base Build 6589591326:	-0.3%
Covered Lines:	381
Relevant Lines:	390

💛 - Coveralls

Uxio0 marked this pull request as ready for review

October 20, 2023 12:18

mmv08 requested review from a team, rmeissner, nlordell, akshay-ap and mmv08 and removed request for a team

October 20, 2023 12:36

nlordell reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol

		@@ -0,0 +1,129 @@
		// SPDX-License-Identifier: LGPL-3.0-only

Collaborator

nlordell Oct 20, 2023

comment: Completely unrelated to this PR, but maybe it would make sense to group all migration contracts in a contracts/migrations root directory? Not suggesting doing this as part of this PR, but if I were to jump into the codebase without any context, I would find looking for migration contracts in a separate root slightly more natural and intuitive. Food for thought...

Member Author

Uxio0 Oct 24, 2023

Makes total sense, I found it weird but I put it on the same directory as the rest of the migrations

nlordell reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol Outdated Show resolved Hide resolved

nlordell reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol Show resolved Hide resolved

nlordell reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol Outdated Show resolved Hide resolved

Uxio0 added 4 commits

October 24, 2023 12:30


          Add contract to migrate a Safe from not L2 to L2

07c8a93

- Related to safe-global/safe-transaction-service#1703


          Fix linting

3ac8ea6


          Add tests

00b1369


          Remove dead code

03bd68d

Uxio0 force-pushed the migrate-l2 branch from a2c87cc to 03bd68d Compare

October 24, 2023 10:31

Member Author

Uxio0 commented Oct 24, 2023 •

edited

Loading

@rmeissner Do you think support for migrating v1.1.1 to L2 is required? (I would need to emit SafeCreation event for those Safes)

mmv08 reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol Outdated Show resolved Hide resolved

contracts/libraries/SafeToL2Migration.sol Show resolved Hide resolved

test/libraries/SafeToL2Migration.spec.ts Outdated Show resolved Hide resolved

Uxio0 and others added 4 commits

October 24, 2023 14:32


          Update contracts/libraries/SafeToL2Migration.sol

c1abd94

Co-authored-by: Mikhail <[email protected]>


          Adding docs

036adfd


          Fix typo

e85b404


          Add test for migrating from v1.4.1 to v1.4.1 L2

9fd1b87

mmv08 approved these changes

View reviewed changes

nlordell approved these changes

View reviewed changes


          Add support to migrate from v1.1.1

00ab5ea

Uxio0 force-pushed the migrate-l2 branch from 6eb48a4 to 00ab5ea Compare

October 25, 2023 10:40

Member Author

Uxio0 commented Oct 25, 2023 •

edited

Loading

@nlordell @mmv08 I added a method and a test for migrating from V1.1.1. In that case 2 more steps need to be performed, setting up the according fallback handler and emitting the SafeSetup event so the backend can index the Safe. There are a lot of 1.1.1 Safes and I think it's worth it to support them.

nlordell reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol

+                      );
+                      ISafe safe = ISafe(address(this));
+                      safe.setFallbackHandler(fallbackHandler);

Collaborator

nlordell Oct 25, 2023

Shouldn't this only happen if the default fallback handler is used? Otherwise it could be unintentionally overwriting a custom fallback handler.

Member Author

Uxio0 Oct 25, 2023 •

edited

Loading

You are right, but:

For the reasons exposed before, it's difficult to know which is the default fallback handler, and more for the 1.1.1 where we didn't do a big follow up on safe-deployments.
I don't think it will be intended for a user to update to 1.3.1/1.4.1 but still keep their old custom fallback handler.
This smart contract is done thinking on get the Safe supported on our UI. If they need to use the UI the official fallback handler is required, otherwise for example offchain messages might not work.
If the user developed a custom fallback handler they should be able to use this contract to set it up back or even a new custom one.

mmv08 reviewed

View reviewed changes

contracts/libraries/SafeToL2Migration.sol Outdated

+                  }
+                  /**
+                   * @notice Migrate from Safe 1.1.1 Singleton to 1.3.1 or 1.4.1 L2

Member

mmv08 Oct 26, 2023

Suggested change

      
                 * @notice Migrate from Safe 1.1.1 Singleton to 1.3.1 or 1.4.1 L2
          
                 * @notice Migrate from Safe 1.1.1 Singleton to 1.3.0 or 1.4.1 L2

Member Author

Uxio0 Oct 26, 2023

Ops

mmv08 approved these changes

View reviewed changes

Uxio0 added 4 commits

October 26, 2023 16:31


          Fix typo

1417a08


          Add modifier only nonce zero

- Hardcode nonce 0 in event instead of nonce - 1, should be more gas efficient


          Add script to deploy L2 migration

7bef949


          Rename deploy migrations script

58ca94e

Uxio0 mentioned this pull request

Support L2 Safes upgraded from non L2 Safes safe-global/safe-transaction-service#1703

Closed

Uxio0 merged commit 482332f into main

18 checks passed

Uxio0 deleted the migrate-l2 branch

October 31, 2023 10:49

Member Author

Uxio0 commented Oct 31, 2023 •

edited

Loading

For context, we tested it on Polygon:

Migration contract was deployed on: https://polygonscan.com/address/0x1Ba30910CE8Eb45b276aa7c89C38FC52228B85CB

2 non L2 Safes were created and were converted to L2 Safes:

0x18D339380c46F431ddbD32b6f4749f6Aca24Be80 : v1.1.1
0x8C852E7Cb94512E6325C7460Fd2A352920717c70 : v1.3.0 (not L2)

Thanks @JagoFigueroa !

Saw-mon-and-Natalie pushed a commit to Saw-mon-and-Natalie/safe-contracts that referenced this pull request


          Add contract to migrate a Safe from not L2 to L2 (safe-global#685)

53b3e7a

* Add contract to migrate a Safe from not L2 to L2
* Related to safe-global/safe-transaction-service#1703

pfedan mentioned this pull request

Technical background for onlyNonceZero in SafeToL2Migration? #720

Closed

gosuto-inzasheru mentioned this pull request

no official deployments for SafeToL2Migration.sol safe-global/safe-deployments#444

Closed

mmv08 pushed a commit that referenced this pull request


          Add contract to migrate a Safe from not L2 to L2 (#685)

8e28148

* Add contract to migrate a Safe from not L2 to L2
* Related to safe-global/safe-transaction-service#1703

mmv08 mentioned this pull request

1.4.1 Feature: Backport migration contracts to 1.4.1 #795

Merged

mmv08 added a commit that referenced this pull request


          1.4.1 Feature: Backport migration contracts to 1.4.1 (#795)

ab1e24e

This PR implements
#781.

It migrates the migration contracts from:
- #685
- #793

I had to refactor tests quite a bit because a lot has changed since the
release of 1.4.1 like we added typechain types, migrated to ethers v6,
etc

---------

Co-authored-by: Akshay <[email protected]>
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
Co-authored-by: Shebin John <[email protected]>
Co-authored-by: Uxío <[email protected]>

dodger213 added a commit to dodger213/safe-smart-contract that referenced this pull request


          1.4.1 Feature: Backport migration contracts to 1.4.1 (#795)

79cdf15

This PR implements
safe-global/safe-smart-account#781.

It migrates the migration contracts from:
- safe-global/safe-smart-account#685
- safe-global/safe-smart-account#793

I had to refactor tests quite a bit because a lot has changed since the
release of 1.4.1 like we added typechain types, migrated to ethers v6,
etc

---------

Co-authored-by: Akshay <[email protected]>
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
Co-authored-by: Shebin John <[email protected]>
Co-authored-by: Uxío <[email protected]>

nlordell added a commit that referenced this pull request


          1.4.1 Feature: Backport migration contracts to 1.4.1 (#795)

db694b9

This PR implements
#781.

It migrates the migration contracts from:
- #685
- #793

I had to refactor tests quite a bit because a lot has changed since the
release of 1.4.1 like we added typechain types, migrated to ethers v6,
etc

---------

Co-authored-by: Akshay <[email protected]>
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
Co-authored-by: Shebin John <[email protected]>
Co-authored-by: Uxío <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

nlordell nlordell approved these changes

mmv08 mmv08 approved these changes

rmeissner Awaiting requested review from rmeissner rmeissner was automatically assigned from safe-global/safe-protocol

akshay-ap Awaiting requested review from akshay-ap akshay-ap was automatically assigned from safe-global/safe-protocol

Labels

None yet